-
Notifications
You must be signed in to change notification settings - Fork 28
Limits Email Sending on Recurring Errors and Configuration Option for Full Backtrace #80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Introduce the one_email_per_occurrence configuration option to limit email notifications to one per occurrence. Updated README and error controller to support the new feature, and added Rake task to handle migrations.
- Converted prev_resolved_at to a datetime from a timestamp in migration - Added a method `should_send_email?` to determine if an email goes instead of a string of conditionals - Cleaned up guard clauses - Improved the datetime comparison in an AR query
The refactor involved removing a segment which forced a specific migration to run in favor of Rails' mechanism of alerting when migrations are pending. The prior method was too rigid and hard-coded. Now, the system still copies the migration over but doesn't automatically run it.
The 'prev_resolved_at' parameter has been removed from the error_params in the errors_controller, and subsequently from the error buttons in the views. Meanwhile, in the subscriber, 'prev_resolved_at' is now updated with the current 'resolved_at' before this is set to nil, maintaining the history of resolution times.
Simplified the condition check whether to send an email in SolidErrors::Occurrence. The method should_send_email? now has guard clauses if SolidErrors does not send emails or if no email recipient is set. Additionally, the instance method one_email_per_occurrence is renamed to one_email_per_occurrence? to follow the naming convention like `send_emails?`.
The config option that limits the email notifications to one email per error occurrence in the SolidErrors has been removed and, instead, that approach is used automatically (sending one per occurrence). Corrections were also made to the post-install message in the gemspec file to remove the ``*and* runs` from the note since we are not auto-running migrations.
…ng process In the 'Occurrence' model, the condition checking if an error has only one occurrence has been simplified by replacing 'count == 1' with 'one?'. In the rake task, the file copying process from Solid Errors to Rails application has been altered so that instead of copying the file directly, it's named with a timestamp before copying to avoid potential naming conflicts. The migration file name was also simplified by removing the timestamp so that we can dynamically add one.
This patch replaces the original file existence check so that it now checks if there are any files with the same original filename (no timestamp) in the entire directory structure under the destination path. This avoids unnecessary file copy operations.
Don't need nested directory in glob
Co-authored-by: Stephen Margheim <stephen.margheim@gmail.com>
Co-authored-by: Stephen Margheim <stephen.margheim@gmail.com>
Co-authored-by: Stephen Margheim <stephen.margheim@gmail.com>
Added instructions to README file for running migration installer after updating the gem. Also, refactored 'solid_errors_tasks.rake' by simplifying the filename generation process during migration. These changes facilitate smoother gem updates and migrations.
This version (x.003) has changes that ensure the migration is copied to the containing application.
Introduced the `full_backtrace` configuration option for more granular control over logging error details. Removed unused migration files and rake tasks to simplify the codebase. Enhanced error handling by adding a backtrace cleaner to filter out irrelevant lines from logs.
Updated solid_errors gem from version 0.6.1 to 0.7.0 in Gemfile.lock. Adjusted the backtrace logic in subscriber.rb to correctly apply the backtrace cleaner only when needed.
Refine backtrace regex to support both single and double quotes. Add Ruby 3.4.0 to CI workflow for testing. Upgrade Nokogiri and SQLite3 to newer versions to ensure compatibility and performance improvements.
The actionmailer dependency was removed from solid_errors in the Gemfile.lock. This change ensures compatibility and reflects the updated requirements of the gem. Corrected an incorrect choice in a merge conflict
Corrected the use of the `error.backtrace` variable to `backtrace` in `lib/solid_errors/subscriber.rb` to ensure proper handling of backtrace data.
…ror updates. - Added `should_send_email?` method for improved email sending logic. - Removed `prev_resolved_at` column from the database schema. - Simplified error occurrence updates by dropping `prev_resolved_at` usage. - Ensured `resolved_at` is reset when sending email notifications.
- Refined `should_send_email?` to check `resolved_at` directly for accuracy. - Updated Gemfile.lock to use `solid_errors` version 0.7.2.
- Adjusted `should_send_email?` to reference `error.resolved_at` for consistency. - Updated Gemfile.lock to use `solid_errors` version 0.7.3. - Incremented gem version to 0.7.3.
- Refined `should_send_email?` to use `error.resolved?` for clarity. - Updated Gemfile.lock to use `solid_errors` version 0.7.4. - Incremented gem version to 0.7.4.
- Fixed incorrect reference of `self` in `send_email`, replacing it with `error` for consistency. - Updated Gemfile.lock to use `solid_errors` version 0.7.5. - Incremented gem version to 0.7.5.
…d_at`, and bump `solid_errors` to 0.8.0 - Refined `should_send_email?` to better handle resolved errors and new occurrences. - Removed `resolved_at` reset logic in `send_email` for simplification. - Introduced `prev_resolved_at` column to track previous resolution timestamps. - Incremented gem version to 0.8.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, split this into 2 PR as there are 2 distinct feature on this PR
| t.text "severity", null: false | ||
| t.text "source" | ||
| t.datetime "resolved_at" | ||
| t.datetime "prev_resolved_at" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont see this being used? to my understanding, the way we are currently checking it is by checking wether resolved_at previously has value or not.
| belongs_to :error, class_name: "SolidErrors::Error" | ||
|
|
||
| after_create_commit :send_email, if: -> { SolidErrors.send_emails? && SolidErrors.email_to.present? } | ||
| after_create :send_email, if: :should_send_email? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think we need to change the after_create_commit.
| end | ||
|
|
||
| def should_send_email? | ||
| return false unless SolidErrors.send_emails? && SolidErrors.email_to.present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a regression, the default is currently we send email whenever an error occured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely and that is exactly what I do not want in my app (an email for every time an error occurs). It would send 20 in instances where an error occurred multiple times (like in a job that gets re-run, etc.).
This is what I need it to do. So, I will close the PR and continue down my fork. Thanks for the review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, this is not a rejection, I'm not even the maintainer :). I just want to see this feature get attention and ultimately merged into the main repo. In term of regression, this is ultimately not me who decide, this gem is pre 1.0 so i guess regression is fine ? Other option is we can have an option to enable this feature.
|
Closing as these changes are suited for my apps and my needs but not the larger project as a whole, so I will simply continue to run off my fork. Thanks for the excellent base of work!! |
|
Oh my gosh, I did not take it that way and apologize for it coming across that way. This was my second PR for the same initial feature (the limited emails per error) and it’s been so long that I realized my local setup had changed and diverged enough from this repo it seemed like my approach wasn’t really a fit with the main repo.
… On Sep 12, 2025, at 1:56 AM, Yana Agun Siswanto ***@***.***> wrote:
@bekicot commented on this pull request.
In app/models/solid_errors/occurrence.rb <#80 (comment)>:
> @@ -45,5 +45,18 @@ def should_clear_resolved_errors?
true
end
+
+ def should_send_email?
+ return false unless SolidErrors.send_emails? && SolidErrors.email_to.present?
Hey, this is not a rejection, I'm not even the maintainer :). I just want to see this feature get attention and ultimately merged into the main repo. In term of regression, this is ultimately not me who decide, this gem is pre 1.0 so i guess regression is fine ? Other option is we can have an option to enable this feature.
—
Reply to this email directly, view it on GitHub <#80 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAC6VQDBKEH3F4ABNO652L3SJ4CXAVCNFSM6AAAAAB73R4YW6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTEMJVGI3DANZXGY>.
You are receiving this because you modified the open/close state.
|
This PR contains two potential additions to the parent repository for Solid Errors:
New configuration option to show the full backtrace or to only show project entries
I've run into issues where I have errors but the backtrace only shows entries/lines from Gems and none from the project itself. I often dislike this added noise and prefer on some applications to ignore GEM paths in the backtrace.
Added
mattr_accessor :full_backtraceand it defaults to false (that is will not show the GEMS in the backtrace). The specific massaging of the backtrace:Limits email delivery to the first occurrence or the first reoccurrence of an error
No emails will be delivered after the first error occurrence until an error is resolved and then reoccurs.
NOTE:
This may not be a desired or mergeable change based on some other work I've seen done on the parent repo in the previous months. Feel free to close as desired. This base of code works great for me and am just offering in the event that it proves useful for the parent repo. Thanks